-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make RemoteDandiset.get_version()
return a VersionInfo
instance with validation error fields
#1210
Conversation
…ith validation error fields
Codecov ReportBase: 89.19% // Head: 89.22% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1210 +/- ##
==========================================
+ Coverage 89.19% 89.22% +0.03%
==========================================
Files 76 76
Lines 9487 9488 +1
==========================================
+ Hits 8462 8466 +4
+ Misses 1025 1022 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
dandi/dandiapi.py
Outdated
@@ -650,6 +650,23 @@ def __str__(self) -> str: | |||
return self.identifier | |||
|
|||
|
|||
class ValidationError(APIBase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class ValidationError(APIBase): | |
class RemoteValidationError(APIBase): | |
""" | |
.. versionadded:: 0.49.0 | |
Validation Error record obtained from a server. To not to be confused with | |
:class:`dandi.validate_types.ValidationResult` which provides richer representation | |
of validation errors. |
as most of the constructs in this file have Remote
in them. (note that sphinx ref to class might be wrong, typing from memory of how those are done)
But I also wonder if may be ValidationResult
could just be used here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if switch to ValidationResult I would just keep all errors (for version and assets) in a single list but with a proper scope
annotation within each ValidationResult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if switch to ValidationResult I would just keep all errors (for version and assets) in a single list
That's not really up to us; the API returns the errors in two separate fields, and it wouldn't be worth the hassle to change that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: f1d752b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, we have those super-powers I would say, but ok -- let's stay close to the original return structure at this level.
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
🚀 PR was released in |
Closes #1206.